Conversation
…ack-overflows-on-recursive-input
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTrack and reuse in-progress RPC message constructions to break recursion when building RPC messages from recursive GraphQL input object types; add tests that generate execution plans from recursive schemas and assert pointer reuse and safe string formatting of recursive messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go`:
- Around line 3-8: The import block currently mixes the local repo import
"github.com/wundergraph/graphql-go-tools/v2/pkg/astparser" with third-party
imports; split imports into separate groups so gci/golangci-lint passes: keep
standard library ("testing") in the first group, third-party
("github.com/stretchr/testify/require") in the second, and the local repo import
("github.com/wundergraph/graphql-go-tools/v2/pkg/astparser") in its own third
group; update the import block in execution_plan_recursive_test.go accordingly.
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan.go`:
- Around line 689-707: The recursive pointer reuse of RPCMessage via
r.visitedInputTypes prevents planning recursion but breaks
RPCExecutionPlan.String(): formatRPCMessage() traverses field.Message with no
cycle detection and will overflow on recursive inputs; update
RPCExecutionPlan.String() (or formatRPCMessage()) to maintain a visited set
(map[*RPCMessage]struct{} or map[string]struct{}) and check it before recursing
into field.Message, returning a safe placeholder (e.g., "<recursive>") or
short-circuiting when the message pointer/name was already seen; ensure you
reference RPCMessage, field.Message, formatRPCMessage, and visitedInputTypes to
locate the code paths to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6258f461-b27e-4612-b3b4-d13163fda81c
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go (1)
62-63: Prefer order-independent field lookup in assertions.Line 62, Line 113, and Line 161 rely on fixed
Fieldsindices. That can make tests flaky if field ordering changes while semantics stay correct. Consider resolving fields byJSONPathbefore validating cycle references.Also applies to: 113-114, 161-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go` around lines 62 - 63, The assertions use positional indices into conditionsField.Message.Fields (assigned to andField and orField) which is fragile; instead locate fields by their JSON/name (e.g., find the Field with JSONName or Name "and" and the one "or") before validating cycle references. Update the test helper logic around conditionsField.Message.Fields, andField, and orField (and the same lookups later in the file where fields are indexed) to perform an order-independent lookup (search the slice for the matching field name/JSONPath) and then run the existing cycle-reference assertions against the found field objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go`:
- Around line 102-104: Two subtests retrieve call := plan.Calls[0] but omit
asserting the RPC method; add assertions for call.MethodName in the 2nd and 3rd
subtests right after call is assigned (same place the first subtest checks
MethodName). Specifically, update the subtests that use plan.Calls and call to
include require.Equal(t, "<expected RPC method string>", call.MethodName) or
require.NotEmpty(t, call.MethodName) as appropriate so regressions in
mapping/planning fail at the root cause; reference the local variables plan,
call and the field MethodName when adding the assertion.
---
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go`:
- Around line 62-63: The assertions use positional indices into
conditionsField.Message.Fields (assigned to andField and orField) which is
fragile; instead locate fields by their JSON/name (e.g., find the Field with
JSONName or Name "and" and the one "or") before validating cycle references.
Update the test helper logic around conditionsField.Message.Fields, andField,
and orField (and the same lookups later in the file where fields are indexed) to
perform an order-independent lookup (search the slice for the matching field
name/JSONPath) and then run the existing cycle-reference assertions against the
found field objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 800847ad-2ccc-4c8c-8984-99cd84e89414
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go`:
- Around line 46-49: Update the stale test comment and strengthen the assertion:
change the comment above the call to plan.String() to note that formatRPCMessage
now tracks visited pointers (so cycle detection exists) instead of saying it has
none, then replace the weak require.NotEmpty(t, result) with a stronger
assertion (e.g., assert the output contains expected markers or has length above
a reasonable threshold) to catch recursion regressions; reference the
plan.String() call and the formatRPCMessage behavior when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86d4c97c-c972-4f39-a0f6-fc9cd9de2ada
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go (1)
46-49:⚠️ Potential issue | 🟡 MinorUpdate stale formatter comment and strengthen the recursion assertion.
Line 46 says there is no cycle detection, which is now misleading. Also, Line 49 (
require.NotEmpty) is too weak to catch recursion-format regressions.Suggested patch
- // String() traverses the plan via formatRPCMessage which has no cycle detection. - // This should not stack overflow. + // String() traverses the plan via formatRPCMessage with cycle detection. + // This should not stack overflow and should mark recursive references. result := plan.String() - require.NotEmpty(t, result) + require.Contains(t, result, "<recursive: ConditionsInput>")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go` around lines 46 - 49, Update the stale comment to note that formatRPCMessage now includes cycle detection, and strengthen the assertion by replacing require.NotEmpty(t, result) with a check that the call to plan.String() completes and produces a reasonably bounded output (e.g., require.NotEmpty plus require.Less(t, len(result), 10000) or equivalent) to catch runaway recursion/format regressions; refer to plan.String() and formatRPCMessage when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go`:
- Around line 46-49: Update the stale comment to note that formatRPCMessage now
includes cycle detection, and strengthen the assertion by replacing
require.NotEmpty(t, result) with a check that the call to plan.String()
completes and produces a reasonably bounded output (e.g., require.NotEmpty plus
require.Less(t, len(result), 10000) or equivalent) to catch runaway
recursion/format regressions; refer to plan.String() and formatRPCMessage when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92857290-8f4e-4569-b70b-6261b63ba9f4
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_recursive_test.go
…ack-overflows-on-recursive-input
🤖 I have created a release *beep* *boop* --- ## [2.0.0](v2.0.0-rc.270...v2.0.0) (2026-04-27) ### Features * support costs on arguments of directives ([#1465](#1465)) ([2eca1ab](2eca1ab)) ### Bug Fixes * grpc datasource stack overflows on recursive input ([#1466](#1466)) ([eba0f58](eba0f58)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
When using a grpc datasource in combination with a schema using recursive input types, the datasource keeps creating new rpc message fields during planning until it hits a stack overflow.
results in fatal error
This pull request fixes it by reusing a message for types we already visited.
@coderabbitai summary
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.